-
Notifications
You must be signed in to change notification settings - Fork 4k
.Net: Support records with no key, fix issue with Relevance #6393
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
dotnet/src/Connectors/Connectors.Memory.AzureCosmosDBNoSQL/AzureCosmosDBNoSQLMemoryStore.cs
Outdated
Show resolved
Hide resolved
dotnet/src/Connectors/Connectors.Memory.AzureCosmosDBNoSQL/AzureCosmosDBNoSQLMemoryStore.cs
Show resolved
Hide resolved
629bf78
to
af4248e
Compare
@dmytrostruk - can you take another look here? I restricted the provider to only use |
dotnet/src/Connectors/Connectors.Memory.AzureCosmosDBNoSQL/AzureCosmosDBNoSQLMemoryStore.cs
Outdated
Show resolved
Hide resolved
dotnet/src/Connectors/Connectors.Memory.AzureCosmosDBNoSQL/AzureCosmosDBNoSQLMemoryStore.cs
Show resolved
Hide resolved
…reCosmosDBNoSQLMemoryStore.cs Co-authored-by: Dmytro Struk <13853051+dmytrostruk@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small updates on constant naming. We use PascalCase for constants:
https://learn.microsoft.com/en-us/dotnet/csharp/fundamentals/coding-style/identifier-names#naming-conventions
dotnet/src/Connectors/Connectors.Memory.AzureCosmosDBNoSQL/AzureCosmosDBNoSQLMemoryStore.cs
Outdated
Show resolved
Hide resolved
dotnet/src/Connectors/Connectors.Memory.AzureCosmosDBNoSQL/AzureCosmosDBNoSQLMemoryStore.cs
Outdated
Show resolved
Hide resolved
dotnet/src/Connectors/Connectors.Memory.AzureCosmosDBNoSQL/AzureCosmosDBNoSQLMemoryStore.cs
Outdated
Show resolved
Hide resolved
dotnet/src/Connectors/Connectors.Memory.AzureCosmosDBNoSQL/AzureCosmosDBNoSQLMemoryStore.cs
Outdated
Show resolved
Hide resolved
dotnet/src/Connectors/Connectors.Memory.AzureCosmosDBNoSQL/AzureCosmosDBNoSQLMemoryStore.cs
Outdated
Show resolved
Hide resolved
…t#6393) ### Motivation and Context When using SemanticTextMemory.SaveReferenceAsync, no value is passed for the `Key` to use. In the Cosmos DB Memory Connector, we treat the "Key" as the Id and PartitionKey, and so it has to have a value. Address this by generating a `GUID` to use if none are provided. ### Description * Assign a GUID key when the passed in `MemoryRecord.Key` is null or empty so that CosmosDB always has a valid PartitionKey. * While adding test coverage for this I noticed two things: 1. `withEmbeddings` is ignored in `AzureCosmosDBNoSQLMemoryStore.GetAsync`. It's not clear whether it's worth changing this, as point reads like the current one tend to be less expensive than queries in Cosmos DB. 2. The SimilarityScore returned from `GetNearestAsync` wasn't actually a similarity, it was a cosine distance. Normalize that. Fixes microsoft#6379. ### Contribution Checklist - [x] The code builds clean without any errors or warnings - [x] The PR follows the [SK Contribution Guidelines](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md) and the [pre-submission formatting script](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts) raises no violations - [x] All unit tests pass, and I have added new tests where possible - [x] I didn't break anyone 😄 --------- Co-authored-by: Dmytro Struk <13853051+dmytrostruk@users.noreply.github.com>
Motivation and Context
When using SemanticTextMemory.SaveReferenceAsync, no value is passed for the
Key
to use. In the Cosmos DB Memory Connector, we treat the "Key" as the Id and PartitionKey, and so it has to have a value. Address this by generating aGUID
to use if none are provided.Description
MemoryRecord.Key
is null or empty so that CosmosDB always has a valid PartitionKey.withEmbeddings
is ignored inAzureCosmosDBNoSQLMemoryStore.GetAsync
. It's not clear whether it's worth changing this, as point reads like the current one tend to be less expensive than queries in Cosmos DB.GetNearestAsync
wasn't actually a similarity, it was a cosine distance. Normalize that.Fixes #6379.
Contribution Checklist